Conversation
confunguido
force-pushed
the
gce-abc-mrp-example
branch
from
402decc to
2cc34fb
Compare
KOVALW
left a comment
KOVALW
left a comment
There was a problem hiding this comment.
Some project file management comments and a discussion on whether or not and how to allow implementation of kwargs for .run()
There was a problem hiding this comment.
This file should be placed at packages/example_model/src/example_model/calibrate.py so that it can be run using the command uv run python -m example_model.calibrate
| ) | ||
|
|
||
| def run(self): | ||
| def run(self, **kwargs: Any): |
There was a problem hiding this comment.
If kwargs are being passed to run (requiring all users to add kwargs to their particles_to_params functions), then they should also be passed to the outputs_to_distance function to allow for adding in runtime changes.
We would also need to flag if any members of kwargs are properties of the sampler, since a user may attempt to set those using run. We don't overwrite any sampler properties from the kwargs of .run(), so we should at least for now throw an error that you cannot pass sampler properties here.
For example, I could easily see someone writing
sampler.run(tolerance_values=[10,5,1])if run() accepts **kwargs, but we don't have any feature that overwrites experiment level parameters from .run(), so this might lead someone to have unexpected results. In order to accept **kwargs here, I would say we need to choose one of two options:
- Override sampler properties if they are a match in kwargs
- Throw an error that a key word argument matches a sampler property and abort the run
There was a problem hiding this comment.
For now I've added a suggested commit that goes with option 2.
| seed=None, # Propagation of seed must be SeedSequence not int for proper pseudorandom draws | ||
| ) | ||
|
|
||
| sampler.run(base_inputs=default_inputs) |
There was a problem hiding this comment.
I don't love this syntax for the reasons I describe in reference to the sampler kwargs implementation. These could also be declared within particles to params instead of called externally
scripts/run_binom_calibration.py
Outdated
| ##===================================# | ||
| # Print IQR of param1 in the posterior particles | ||
| posterior_particles = sampler.get_posterior_particles() | ||
| p_values = [p.state["p"] for p in posterior_particles.all_particles] |
There was a problem hiding this comment.
Flagging that it would be nice to have convenience functions for something like posterior_particles.p that would call a property for the distribution of parameter values in a particle population
KOVALW
force-pushed
the
gce-abc-mrp-example
branch
from
f274c95 to
378f79f
Compare
KOVALW
left a comment
KOVALW
left a comment
There was a problem hiding this comment.
With my suggested changes I would say that this is good to merge, just covering a couple blind spots with kwargs and rebased to the changes on my branch
* abcsampler example with binom model using mrp * wtk suggested edits * fix particle state call for rebase --------- Co-authored-by: KOVALW <AD71@cdc.gov>
3 participants
No description provided.